fix: don't desync rxSeq on a SeqCrypt MAC failure#2
Conversation
decrypt() advanced rxSeq by 1 on a MAC failure. Each SAKE direction uses a fixed sequence parity (Session seeds clientCrypt=0 even, serverCrypt=1 odd) and the parity bit is not carried on the wire -- the receiver rebuilds the full sequence from its own rxSeq (seq = rxSeq + 2*delta). A +1 nudge on failure therefore flips rxSeq's parity, so every subsequent packet reconstructs to the wrong sequence: the next genuine packet fails to authenticate, and rxSeq keeps drifting on each failure until delta exceeds MAX_RX_DELTA and valid packets are rejected as forgeries -- wedging the session into a re-handshake. Leave rxSeq unchanged on failure instead. The existing reorder-window delta already re-syncs on the next packet, and rxSeq can never run ahead of the sender, so it cannot desync. The failing-message hex in MacFailureException is unchanged. Adds macFailureDoesNotDesyncSubsequentDecrypt: a corrupted packet is rejected with rxSeq left untouched, and the next genuine packet still decrypts. This test fails against the previous rxSeq + 1 behaviour.
|
@palmarci flagging this for your review since it touches your Short version: the I kept your Full disclosure: I do have merge access on this repo, so I could merge this myself — but I don't think I should land a change to your protocol code without your review, so I'm leaving it to you. On that note, now that JavaSake is picking up real downstream consumers, it might be worth adding branch protection on |
2 participants
Summary
SeqCrypt.decrypt()advancesrxSeqby 1 on a MAC failure (rxSeq = macOk ? seq + 2 : rxSeq + 1, added in 07346cc). That desyncs the receiver for this cipher's sequencing scheme.Each SAKE direction uses a fixed sequence parity —
SessionseedsclientCryptat0(even) andserverCryptat1(odd), and every message advances by 2. The parity bit is not carried on the wire: the trailer holds(seq >>> 1) & 0xFF, and the receiver rebuilds the full sequence from its own counter asseq = rxSeq + 2*delta. So the reconstructed sequence's parity comes entirely fromrxSeq.A
+1on failure flips that parity. The consequences:rxSeqkeeps drifting by 1 on each failure untildeltaexceedsMAX_RX_DELTA (128), at which point valid packets are rejected as forgeries and the session can only recover with a full re-handshake.This is a regression versus the prior behaviour (which left
rxSequntouched on failure and self-recovered through the existing reorder window), and it diverges fromPythonSake, which recovers by brute-forcing the real sequence rather than a fixed nudge.Fix
Leave
rxSequnchanged on a MAC failure. The reorder-window delta already re-syncs on the next packet, and becauserxSeqnever advances past a confirmed packet it can't run ahead of the sender, so it can't desync. The failing-message hex inMacFailureException(d78ff25) is kept.Test
Adds
macFailureDoesNotDesyncSubsequentDecrypt: a corrupted packet is rejected withrxSeqleft untouched, then the next genuine packet still authenticates and decrypts. It fails against the previousrxSeq + 1behaviour and passes with this change.:lib:testandspotlessCheckare green.Verification
Beyond the unit test: driving a live encrypt/decrypt pair through one corrupted packet, the
+1code flippedrxSeq2 → 3 and the next valid packet failed to decrypt; with this changerxSeqstays 2 and the next valid packet decrypts cleanly.Context
Found while pulling these commits into a downstream consumer (GlycemicGPT's vendored SAKE driver). We're carrying this exact fix as a temporary local patch and will re-vendor once it lands here so the two stay in sync. Happy to adjust the approach — e.g. porting PythonSake's brute-force recovery instead — if you'd prefer that direction.